-
Notifications
You must be signed in to change notification settings - Fork 466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update/Fix Pipeline Mixins and ORT Pipelines #2021
Conversation
…ace/optimum into auto-diffusion-pipeline
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
…eproducibility and comparaison tests (7 failed, 35 passed)
b28ff5a
to
dceccca
Compare
Co-authored-by: Ella Charlaix <[email protected]>
I made sure the model also accepts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @IlyasMoutawwakil for raising and working on this PR, it's brilliant! I'm so inspired to improve the diffusers support in Neuron as well!
components = { | ||
"vae": self.vae, | ||
"unet": self.unet, | ||
"text_encoder": self.text_encoder, | ||
"text_encoder_2": self.text_encoder_2, | ||
"safety_checker": self.safety_checker, | ||
"image_encoder": self.image_encoder, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe adding a test leveraging from_pipe() to ensure that we have enough members in the components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And for making this to work you might need to allow passing ORTModelXXX to __init__()
instead of just bare ort session. Never mind, it's a small nit, we can put it as todo anyways!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this one will need a bit more work for compatibility, I can do it in another PR.
as you said it especially requires passing models and not sessions, which might require deprecating some stuff.
I uploaded a model that was exported using optimum 1.22 : I also added the same checks in optimum-intel for missing attributes in model configs, that way even older versions can still work (e.g. I also removed the need for separate |
@@ -278,14 +284,26 @@ def _from_pretrained( | |||
else: | |||
submodels[submodel] = load_method(model_save_path) | |||
|
|||
return cls( | |||
# same as DiffusionPipeline.from_pretraoned, if called directly, it loads the class in the config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smol typo here
What does this PR do?
This PR allows for using the same modeling in diffusers for ORT diffusion pipelines without maintaining custom mixins.
It also fixes the issues in output reproducibility and numeric consistency vs diffusers observed in #1960.
Breaking changes:
We also conduct a benchmark to prove this change doesn't hurt perf: #2021 (comment)
We also show how reproducibility is impossible with an onnx model that performs random numbers generation at export-time: #2021 (comment)
Before submitting
Who can review?